Add way to customize environmentCheck Actions QL#22173
Conversation
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
Thank you very much for doing this! It is highly appreciated!
Would it be possible to add some tests as well?
Note, that for testing the Yaml alternative it is possible to add an .ext.yml file next to the .ql test file - it just needs to have the same base name as the test file. That is, if there is a test named test.ql then adding test.ext.yml makes it possible to add tuples to the extensible predicates that only applies for test.ql.
| EnvironmentCheck() { | ||
| exists(string selected | | ||
| selectDeploymentEnvironmentDataModel(selected) and | ||
| if selected = "EnvironmentCheckMaD" |
There was a problem hiding this comment.
Thank you for looking into this!
I am not as such familiar with the actions QL implementation, so bear with me 😄
As far as I understand the idea of the pull request is to make it configurable which Environment instances that are added as ControlChecks. The PR implements support for this in two ways
- Either by supplying environment names as tuples in
enabledDeploymentEnvironmentDataModelOR - By extending the abstract class
CustomEnvEnable.
On top of that there is an extensible predicateselectDeploymentEnvironmentDataModelused to decide, which of the above strategies to use (if any).
A couple of questions:
- Would it suffice to only have the MaD variant? The Yaml files are loaded dynamically and only supporting a single way of doing it will make it easier to maintain.
- What is the reason for also having the abstract class way of doing it?
| # The possible values this can take are: | ||
| # EnvironmentCheckCustomQL - for a custom QL implementation for the sufficient EnvironmentCheck. | ||
| # EnvironmentCheckMaD - for a custom MaD list of deployment environments that are relevant to a database. | ||
| - [""] No newline at end of file |
There was a problem hiding this comment.
With the current logic in EnvironmentCheck, the extensible predicate selectDeploymentEnvironmentDataModel needs to contain at least one "dummy" tuple - in this case "" - otherwise no Environment instances will be added to ControlCheck.
Perhaps, consider letting keeping the extensible predicate selectDeploymentEnvironmentDataModel empty as default (the logic in EnvironmentCheck needs to be adjusted accordingly).
There was a problem hiding this comment.
Comment is only relevant in case we need to keep both the Yaml and abstract class option.
Otherwise, we can consider assuming that if enabledDeploymentEnvironmentDataModel contains any tuples, then it is exactly those environments that we would like to add to ControlCheck (this removes the need for selectDeploymentEnvironmentDataModel)
| data: | ||
| - [""] No newline at end of file |
There was a problem hiding this comment.
| data: | |
| - [""] | |
| data: [] |
| @@ -1 +1,3 @@ | |||
| import codeql.actions.Ast | |||
|
|
|||
| abstract class CustomEnvEnable extends Environment { } | |||
2 participants
Add implementation for technique to customize
EnvironmentCheckin actions queriesCurrently there is no way to customize Actions query "sanitizers" ie
ControlChecks. This PR adds a sample mechanism that uses 1) a specifier in MaD that designates which technique to use (the default is no customization) and 2) a mechanism that fills the definition of EnvironmentCheck with either i) a QL custom definition or ii) a MaD custom definition.The purpose of this is to allow for checks to be selectively turned off or narrowed based on real info related to deployment environments in repos (this would need to be externally supplied) .
This PR is similar to and borrows ideas from this PR.
The intention of this work would be to potentially apply this style to any check type or other sanitizer types in other languages, if the mechanisms used here are found to be suitable.
One important consideration of this work is that for Actions there is currently no Customizations.qll file. Either this PR should also add one , or it should be carefully considered if the current
actions.qllusage for a similar intended edit location is alright to do (probably the former would be more ideal).